Repository navigation
Ask Composer's autoloaders for the file instead of letting them include it - #6431
Conversation
2eb5402 to
3f90b11
Compare
3f90b11 to
1f51101
Compare
1f51101 to
5ea1f21
Compare
5ea1f21 to
80d5c78
Compare
|
This pull request has been marked as ready for review. |
|
The fix works from source, but in the phar the new fast path never runs, so the fatal from phpstan/phpstan#15184 is still there. php-scoper prefixes the new import. The phar that CI built for this PR (artifact The project's autoloader is an unprefixed Repro on macOS, PHP 8.5.8: a project with
On the PR head from source:
All five red checks are also red on #6564, another 2.2.x PR. I did not run the Windows setup from the issue, and I did not measure performance. |
80d5c78 to
fb40c09
Compare
|
@SanderMuller Thanks for finding the phar issue. I rebased and updated the commit to add In regards to performance, two A/B runs each of PHPStan's own |
fb40c09 to
bed3f0c
Compare
SanderMuller
left a comment
There was a problem hiding this comment.
Thanks, that fixes it. With the phar CI built from bed3f0c, the php-standard-library/type repro now reports class.notFound instead of the fatal, with and without OPcache. The new build test fails without the entry, and the phar test fails without the :// guard.
On a real project of about 4,500 files the reported errors are identical. CPU did not go up: median 140.2s before and 131.4s after, over 3 cold runs each. The full suite and make phpstan pass locally. The red Checksum PHAR job is expected here: the only file that differs between the base and PR checksum phars is AutoloadSourceLocator.php.
bed3f0c to
34f08f3
Compare
b452f74 to
2c905f7
Compare
…de it AutoloadSourceLocator finds the file that declares a class by running the autoloaders inside FileReadTrapStreamWrapper, which serves an empty script for each file they include. When OPcache already holds the script, the include runs the cached copy instead, so a file the process already ran executes again. A file declaring a function then dies with "Cannot redeclare". That is the function-per-file layout of php-standard-library, whose "files" autoload loads paths that its PSR-4 prefix also resolves to. Before running a Composer ClassLoader in the trap, ask its findFile() for the path, so nothing is included. Other autoloaders, and paths behind a stream wrapper such as the phar's own phar://, still go through the trap. In the phar, php-scoper prefixes the ClassLoader import but Box leaves the class unprefixed, so AutoloadSourceLocator.php joins the files where scoper.inc.php removes that prefix again. Closes phpstan/phpstan#15184 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2c905f7 to
44d3c5d
Compare
There was a problem hiding this comment.
🟢 Approval recommended
No blocking issues remain; the file-only fixture bypasses result caching, so both regression invocations perform analysis.
0 open findings
What changed in this PR
Avoids OPcache redeclaration crashes by locating Composer-managed source files without including them.
Changes:
- Uses Composer’s
findFile()while preserving autoloader order and stream-wrapper fallback. - Adjusts PHAR scoping for the Composer class reference.
- Adds regression coverage for debug and spawned-worker analysis.
| File | Description |
|---|---|
| src/Reflection/BetterReflection/SourceLocator/AutoloadSourceLocator.php | Resolves Composer paths without inclusion. |
| e2e/bug-15184/test.php | Exercises a function name used as a class. |
| e2e/bug-15184/src/Type/optional.php | Supplies the function-only fixture. |
| e2e/bug-15184/phpstan.dist.neon | Requires the expected unknown-class diagnostic. |
| e2e/bug-15184/composer.json | Configures overlapping files and PSR-4 autoloading. |
| e2e/bug-15184/.gitignore | Excludes generated dependencies. |
| compiler/build/scoper.inc.php | Preserves the unprefixed Composer reference. |
| .github/workflows/e2e-tests.yml | Runs both regression modes with OPcache available. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
staabm
left a comment
There was a problem hiding this comment.
I verified the test and I think it makes sense.
Co-authored-by: Markus Staab <maggus.staab@googlemail.com>
|
Thank you! |
AutoloadSourceLocatorfinds which file declares a class by running the registered autoloaders behindFileReadTrapStreamWrapper, which records the path anincludereached for and serves an empty script in its place. That only shadows the real file while the compiler asks the wrapper for the contents. When OPcache already holds the script, it does not ask, and the file runs a second time:That is fatal for any file that declares a function, which is how
php-standard-libraryandazjezz/psllay out their code: one function per file. Theirfiles-autoload bootstrap has already loaded every path that their PSR-4 prefix also resolves to.2.2.13 exposed this by starting spawned workers with OPcache on (
ProcessHelper::resolveWorkerIniEntries()), and it still reproduces on 2.3.1. Windows has nopcntl_fork(), so its workers are always spawned.Fix
Before an autoloader that is a Composer
ClassLoaderruns inside the trap,AutoloadSourceLocatorasks itsfindFile()for the path.findFile()answers with the same pathloadClass()would include, without running anything, so nothing is compiled and no cache is consulted. The check sits in the existing loop over the autoloaders, so registration order is unchanged, and every other autoloader still runs in the trap as before.findFile()joins the mapped prefix to the rest of the name, so its answer can contain../segments and mixed separators. It is resolved withrealpath(), as an include would resolve it, becauselocateIdentifier()compares the located path withReflectionClass::getFileName().A path behind a stream wrapper is left to the trap, because
realpath()cannot resolve it. The phar's own autoloader is such aClassLoader: it mapsPHPStan\tophar://.../src.In the phar
php-scoper prefixes the new
use Composer\Autoload\ClassLoader;, but Box leaves the class itself unprefixed, so in the phar theinstanceofwould never match.compiler/build/scoper.inc.phpalready removes that prefix in a fixed list of files, andAutoloadSourceLocator.phpis now on that list.Checksum PHAR is expected to fail, because this PR changes how that one file is scoped.
Tests
e2e/bug-15184has the same layout as the report: a function-per-file module loaded through Composer'sfilesautoload, in a directory its PSR-4 prefix also maps, and code that uses the function's name as a class. It runs with--debugand withpcntl_forkdisabled. Forked workers switch OPcache off, so a default run on Linux would not reach the bug. On Windows, the spawned-worker run crashes withCannot redeclare functionon 2.3.x and passes with this PR.The phar case needs no new test: the existing extension-tests jobs cover it. They failed with
Call to method yes() on an unknown class PHPStan\TrinaryLogic.on an earlier revision of this PR that had no fallback for paths behind a stream wrapper.Also checked by hand on Windows with PHP 8.5, against a project on
php-standard-library/typethat analysesreturn $o instanceof \Psl\Type\optional;:Cannot redeclare function Psl\Type\optional()class.notFoundon theinstanceofline, no fatalAlternatives considered
Earlier revisions of this PR kept the include and tried to make it safe. The first called
opcache_invalidate()from the trap'sstream_open()for the path being opened. Its CI run was cut short by the next push, so it was neither confirmed nor ruled out. The next refused the open for a file already inget_included_files(), and failed the Symplify integration job.#6742 invalidates every included file before any autoloader runs, which also covers autoloaders that are not a Composer
ClassLoader. This PR does not cover those. The two approaches could be combined, with invalidation only before the autoloaders that are not aClassLoader.Closes phpstan/phpstan#15184
🤖 Generated with Claude Code